Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lib: remove startsWith/endsWith primordials for char checks #55407

Merged
merged 5 commits into from
Oct 19, 2024
Merged

lib: remove startsWith/endsWith primordials for char checks #55407

merged 5 commits into from
Oct 19, 2024
+17 −22

Conversation

gurgunday
Copy link
Contributor

@gurgunday gurgunday commented Oct 16, 2024 •
edited
Loading

Removes primordials when we're only checking for one character and leans on JavaScript syntax as much as possible for performance

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/loaders
  • @nodejs/net
  • @nodejs/url
  • @nodejs/web-standards

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 16, 2024
@gurgunday gurgunday changed the title chore: remove startsWith/endsWith primordials for char checks lib: remove startsWith/endsWith primordials for char checks Oct 16, 2024
@panva
Copy link
Member

panva commented Oct 16, 2024

test/fixtures/wpt/resources/webidl2/lib/webidl2.js

This is an upstream repository resource, please remove the change.

ljharb
ljharb approved these changes Oct 16, 2024
View reviewed changes
lib/internal/modules/esm/fetch_module.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/resolve.js Show resolved Hide resolved
anonrig
anonrig reviewed Oct 16, 2024
View reviewed changes
lib/internal/modules/esm/fetch_module.js Outdated Show resolved Hide resolved
lib/internal/process/per_thread.js Outdated Show resolved Hide resolved
lib/internal/process/pre_execution.js Show resolved Hide resolved
lib/internal/modules/esm/fetch_module.js Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Oct 16, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.40%. Comparing base (fa8f149) to head (0bd51d5).
Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/modules/esm/fetch_module.js 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #55407      +/-   ##
==========================================
- Coverage   88.41%   88.40%   -0.02%     
==========================================
  Files         652      653       +1     
  Lines      186918   187595     +677     
  Branches    36072    36113      +41     
==========================================
+ Hits       165270   165839     +569     
- Misses      14891    14975      +84     
- Partials     6757     6781      +24
Files with missing lines Coverage Δ
lib/_http_agent.js 98.17% <100.00%> (ø)
lib/internal/modules/esm/resolve.js 96.54% <100.00%> (+<0.01%) ⬆️
lib/internal/modules/helpers.js 98.80% <100.00%> (-0.20%) ⬇️
lib/internal/process/per_thread.js 99.30% <100.00%> (-0.01%) ⬇️
lib/internal/process/pre_execution.js 90.41% <100.00%> (-0.62%) ⬇️
lib/internal/repl/utils.js 95.72% <100.00%> (-0.02%) ⬇️
lib/internal/url.js 98.04% <100.00%> (+0.08%) ⬆️
lib/internal/util/inspect.js 99.95% <100.00%> (-0.05%) ⬇️
lib/repl.js 94.96% <100.00%> (ø)
lib/internal/modules/esm/fetch_module.js 42.85% <0.00%> (-0.53%) ⬇️

... and 100 files with indirect coverage changes

@gurgunday
Copy link
Contributor Author

@aduh95 can you take a look?

congrats on the release btw :)

mcollina
mcollina approved these changes Oct 17, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@legendecas legendecas mentioned this pull request Oct 18, 2024
Closed
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63178/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63185/

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 18, 2024
@gurgunday
Copy link
Contributor Author

gurgunday commented Oct 19, 2024 •
edited
Loading

@aduh95 will this merge? Or is there an issue

I got some other changes on the way but they will conflict with this one

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 19, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 19, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/55407
✔  Done loading data for nodejs/node/pull/55407
----------------------------------- PR info ------------------------------------
Title      lib: remove startsWith/endsWith primordials for char checks (#55407)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     gurgunday:improve-startswith -> nodejs:main
Labels     lib / src, author ready, needs-ci, commit-queue-squash
Commits    5
 - lib: remove startsWith/endsWith primordials for char checks
 - lint: remove unused vars
 - revert: webidl2 change
 - use .length to check for empty string
 - add assert to `addNumericSeparator`
Committers 1
 - Gürgün Dayıoğlu <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/55407
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mattias Buelens <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55407
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mattias Buelens <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - add assert to `addNumericSeparator`
   ℹ  This PR was created on Wed, 16 Oct 2024 16:53:41 GMT
   ✔  Approvals: 5
   ✔  - Jacob Smith (@JakobJingleheimer): https://github.com/nodejs/node/pull/55407#pullrequestreview-2373132270
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/55407#pullrequestreview-2374915750
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/55407#pullrequestreview-2375049155
   ✔  - Mattias Buelens (@MattiasBuelens): https://github.com/nodejs/node/pull/55407#pullrequestreview-2375222681
   ✔  - Ethan Arrowood (@Ethan-Arrowood): https://github.com/nodejs/node/pull/55407#pullrequestreview-2376217715
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-10-18T16:16:36Z: https://ci.nodejs.org/job/node-test-pull-request/63185/
- Querying data for job/node-test-pull-request/63185/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/11416653036

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 19, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 19, 2024
@nodejs-github-bot nodejs-github-bot merged commit 8ed50bc into nodejs:main Oct 19, 2024
75 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 8ed50bc

@gurgunday gurgunday deleted the improve-startswith branch October 19, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

@Ethan-Arrowood Ethan-Arrowood Ethan-Arrowood approved these changes

@mcollina mcollina mcollina approved these changes

@MattiasBuelens MattiasBuelens MattiasBuelens approved these changes

@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes

@aduh95 aduh95 aduh95 approved these changes

@mertcanaltin mertcanaltin mertcanaltin approved these changes

@anonrig anonrig Awaiting requested review from anonrig

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.